fix(spans-migration): add selection params to explore url in frontend #101185
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| params.push('utc=true'); | ||
| } | ||
|
|
||
| selection.projects.forEach(project => { | ||
| params.push(`project=${project}`); | ||
| }); | ||
|
|
||
| selection.environments.forEach(environment => { | ||
| params.push(`environment=${environment}`); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Potential bug: URL parameters are not encoded, leading to broken links for environment names with special characters.
-
Description: The
createExploreUrlfunction constructs URL parameters without encoding them. Since environment names can contain special characters like '&', '=', or spaces, concatenating them directly into the URL string will result in malformed and broken URLs. This impacts the functionality of the 'Explore' links for users with such environment names. -
Suggested fix: Use
encodeURIComponenton the values of the parameters before adding them to the URL string. For example, changeparams.push(environment=${environment});toparams.push(environment=${encodeURIComponent(environment)});.
severity: 3.0, confidence: 5.0
Did we get this right? 👍 / 👎 to inform future reviews.
| if (selection.datetime?.start) { | ||
| params.push(`start=${getUtcDateString(selection.datetime.start)}`); | ||
| } | ||
| if (selection.datetime?.end) { | ||
| params.push(`end=${getUtcDateString(selection.datetime.end)}`); | ||
| } | ||
| if (selection.datetime?.period) { | ||
| params.push(`statsPeriod=${selection.datetime.period}`); | ||
| } | ||
| if (selection.datetime?.utc) { | ||
| params.push('utc=true'); | ||
| } | ||
|
|
||
| selection.projects.forEach(project => { | ||
| params.push(`project=${project}`); | ||
| }); | ||
|
|
||
| selection.environments.forEach(environment => { | ||
| params.push(`environment=${environment}`); | ||
| }); |
There was a problem hiding this comment.
this feels a little janky to me, could we instead parse this URL, something like
const parsedURL = safeURL(baseUrl);
const queryParams = qs.parse(parsedURL?.search ?? '');
// append all the page filters to queryParams - perhaps use an existing util like pageFiltersToQueryParams
return getExploreUrl(queryParams)
| queryParams.aggregateField = queryParams.aggregateField.map(item => | ||
| JSON.parse(item) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: JSON Parsing Errors in createExploreUrl
The createExploreUrl function calls JSON.parse() on aggregateField values without validation or error handling. This can lead to runtime errors and component crashes if the aggregateField parameter contains malformed JSON strings or if array elements are not strings.
add the page params to the transaction widget warning urls on the frontend so they can see the same thing if they haven't submitted their dashboard filters. NOTE: i noticed that now the entire dashboard reloads when changing dashboard filters because the urls have to be re-rendered. This kinda sucks but it will be temporary